Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for EDNS and DNSSEC #803

Merged
merged 3 commits into from
Jan 25, 2016
Merged

Conversation

McStork
Copy link
Contributor

@McStork McStork commented Jan 21, 2016

Library change from gopacket to miekg/dns
  • Modify payload objects and attributes used throughout the dns package
  • FQDNs now terminate with a dot as any absolute domain name should RFC1035
  • Update glide with the miekg/dns library
Also
  • Do a minor cleaning in the function that decodes DNS over TCP
  • Move DnsMessage struct from dns_tcp.go to dns.go
  • Change the flag 'recursion_allowed' to 'recursion_available' DNS Header Flags
  • Change the SOA field that was named 'ttl' to 'minimum' RFC 1035
TODO list
  • Change decode library to miekg/dns
  • EDNS
  • DNSSEC

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

data = rr.IP.String()
case layers.DNSTypeSOA:
// We don't have special handling for this type
logp.Debug("dns", "Unsupported RR type %s", dnsTypeToString(rrType))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the default case set data = x.Rdata? I'm assuming Rdata is raw data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. At first I thought that couldn't be achieved ... because only type RFC3597 has the Rdata field, other types having the fields according to their format.
But a method ToRFC3597 helps fetch this field from any RR type.
With some lines of code (e.g. here), the Rdata is fetched. I'm not sure a simpler solution exists though.

@andrewkroh
Copy link
Member

@McStork, the changes look really solid to me. Thanks

A few things...

  • Re-generate the docs/fields.asciidoc from the modified fields.yml. There is a python script in the scripts directory for this.
  • Update the project changelog to say that we switched to miekg/dns for parsing DNS. And note in the backward compatibility section about the FQDN's ending in dots and the rename of the field.

@andrewkroh
Copy link
Member

@elastic/beats Should the glide.yaml contain the commit hash of github.com/miekg/dns? I think we should include the commit hash so we don't have to guess at what point we took the library, but I don't really know all the details of glide.

@McStork
Copy link
Contributor Author

McStork commented Jan 22, 2016

@andrewkroh Generating fields.asciidoc showed that two fields from ICMP (icmp.request.message, icmp.response.message) are missing from the doc. This affects v1.1 btw. Should a ticket be open?

@andrewkroh
Copy link
Member

Regarding the fields.asciidoc on 1.1. We updated it - #821. Thanks for pointing it out.

Library change from gopacket to miekg/dns:
* Modify payload objects and attributes used throughout the dns package
* FQDNs now terminate with a dot as any absolute domain name should [RFC1035](https://tools.ietf.org/html/rfc1035)
* Update glide with the miekg/dns library

Also:
* Do a minor cleaning in the function that decodes DNS over TCP
* Move DnsMessage struct from dns_tcp.go to dns.go
* Change the flag 'recursion_allowed' to 'recursion_available' [DNS Header Flags](http://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml#dns-parameters-12)
* Fetch Rdata for yet unhandled and true unknowns RR types
* Re-generate the docs/fields.asciidoc
* Update CHANGELOG
* Add commit hash of lib miekg/dns to glide.yaml

Also
* Change the SOA field that was named 'ttl' to 'minimum' [RFC 1035](https://www.ietf.org/rfc/rfc1035.txt)
@andrewkroh
Copy link
Member

LGTM so I am going to merge it now and additional changes can be done is new PR. Thanks

andrewkroh added a commit that referenced this pull request Jan 25, 2016
Change decode library to miekg/dns
@andrewkroh andrewkroh merged commit fe9655c into elastic:master Jan 25, 2016
@McStork
Copy link
Contributor Author

McStork commented Jan 25, 2016

@andrewkroh Ok! Let's do it this way then.

@McStork McStork deleted the dns-miekg branch February 28, 2016 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants